Finish author viewpoint page #11

yukicoder0509 · 2024-11-21T06:15:59Z

I have finished author-viewpoint page and fixed some center layout problems and adjust the height setting of each page.

vercel · 2024-11-21T06:16:04Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 5, 2024 0:33am

src/app/issues/[id]/author/page.tsx

src/components/AuthorViewpoint/FactCard.tsx

src/components/AuthorViewpoint/FactListCard.tsx

src/components/AuthorViewpoint/ViewpointCard.tsx

burnedinthesky · 2024-11-25T08:25:10Z

In addition, per the design file, please remember to add a 6px padding between paragraphs.

burnedinthesky

All previous issues have been resolved except for the paragraph spacing. By paragraph spacing I mean additional padding between paragraphs of text (created by pressing the enter key). Paragraph spacing should be in addition to line height. For example: the distance between text "Line 1 \n Line 2" should be 6px more than "Line 1..... [line-wrap] line 2".

burnedinthesky · 2024-12-02T08:34:54Z

src/app/issues/[id]/author/page.tsx

+    };
+};
+
+type MetadataProps = {


Remove unused props type

burnedinthesky

Sorry for missing this in the last review, but I think there's a major misunderstanding in where the facts in the FactListCard come from. The facts are not automatically imported from the issue (since there may be hundreds if not thousands of facts per issue), but instead the user gets an empty list, and adds facts to the list by referenceing existing facts (via the search bar) or authoring new facts (via the fact creation feature). Please edit your code to reflect this system design.

burnedinthesky · 2024-12-02T08:37:25Z

src/app/issues/[id]/author/page.tsx

+                    <ViewpointCard />
+                </div>
+                <div className="w-1/3">
+                    <FactListCard facts={issue.facts} />


Since there may be hundreds or thousands of facts per issue, we by default will show only an empty list of facts when the user loads the author viewpoint page. Any fact that appears in the reference area must either be there because (a) a user searched for & added it via the search bar or (b) a user created a fact when authoring this viewpoint

burnedinthesky · 2024-12-02T08:44:44Z

src/components/AuthorViewpoint/FactListCard.tsx

+};
+
+export default function FactListCard({ facts }: FactListCardProps) {
+    const [searchData] = useState<string[]>([


Please remember to define the setSearchData returned as well. If eslint warns you for unused variables, consider disabling that rule for this line temporarily (guide to how), or better, move the mock data to the server response for keyword search, and implement the "select fact to reference" function in this PR.

burnedinthesky · 2024-12-02T08:45:46Z

src/components/AuthorViewpoint/FactListCard.tsx

+                    classNames={{
+                        input: "ml-2 bg-transparent text-lg font-normal text-neutral-500 focus-within:outline-b-2 focus-within:border-b-emerald-500 focus-within:outline-none",
+                    }}
+                    placeholder="搜尋 CommonGround"


Add the option to import a fact when nothing is found (Mantine reference)

burnedinthesky · 2024-12-02T08:49:53Z

src/components/AuthorViewpoint/FactListCard.tsx

+                + 92px(FactListCard title and search box) + 16px(FactListCard padding-bottom)*/}
+                <div className="flex flex-col gap-3 pl-7 pr-4">
+                    {facts.map((fact) => (
+                        <div key={fact.id}>


I think the outer div is unnecessary, just <EditViewpointFact key={fact.id} fact={fact}> should do

…/frontend into author-viewpoint

burnedinthesky · 2024-12-07T05:55:40Z

src/components/AuthorViewpoint/FactListCard.tsx

+                    variant="transparent"
+                    leftSection={<PlusIcon className="h-6 w-6" />}
+                    classNames={{
+                        root: "px-0 ml-7 mt-2 text-neutral-600 text-base font-normal hover:text-emerald-500 duration-300",


The current styling results in a padding whether there are facts before this or not. It makes less sense to add a padding here since the searchbar already has a y-padding. Consider moving the Button into the flexbox and utilizing the gap property.

burnedinthesky · 2024-12-07T05:57:41Z

src/app/providers.tsx

@@ -0,0 +1,51 @@
+"use client";


This file should not be included in this branch as the scope of this PR does not include networking

burnedinthesky · 2024-12-07T06:11:03Z

src/components/AuthorViewpoint/EditViewpointFact.tsx

+                <Button
+                    variant="transparent"
+                    classNames={{
+                        root: "float-right pr-1 pl-0 flex opacity-0 group-hover:opacity-100",


Consider using the hidden class instead of setting opacity

To prevent layout shifts when toggling the visibility of an element, instead of using display to show or hide it—which removes the element from the layout entirely—I use the visibility property. This approach visually hides the element while keeping its space reserved in the layout, ensuring a stable and consistent page structure.

burnedinthesky · 2024-12-07T06:12:57Z

src/components/AuthorViewpoint/FactListCard.tsx

+    viewpointFactList,
+    setViewpointFactList,
+}: FactListCardProps) {
+    const [searchData, setSearchData] = useState<Fact[]>(allFacts); // eslint-disable-line


Since both attributes are used, this line shouldn't be ignored by eslint.

burnedinthesky · 2024-12-07T06:15:30Z

src/app/issues/[id]/author/page.tsx

+    const id = params.id;
+
+    return (
+        <main className="mx-auto my-8 w-full max-w-7xl">


Since this page doesn't have much content and is mostly client-side logic, I think we can consider making this page completely client-side rendered and move all login into AuthorViewpointCard.

burnedinthesky · 2024-12-07T06:17:31Z

src/components/AuthorViewpoint/ViewpointCard.tsx

+
+    const onPublish = () => {
+        if (viewpointTitle == "" || contentEmpty) {
+            console.log("Title and Content MUST NOT be empty");


Consider using a toast to display this message to the user as most users won't check the JS console

Furthermore, if the message were to appear in a toast, please translate the message into Chinese.

The console is added to check if the logic works when developing.

burnedinthesky · 2024-12-07T06:19:28Z

src/components/AuthorViewpoint/ViewpointCard.tsx

+                        if (node.className.includes("pt-1.5")) return;
+                        node.classList.add("pt-1.5");
+                    });
+                    setViewpointContent(


Since this trigger are quite frequent, this may result in common costly rerenders that slow the application down. Also, considering that the content of this div is not controlled, I think we can consider moving this event into onBlur

burnedinthesky · 2024-12-07T06:27:07Z

src/components/AuthorViewpoint/FactListCard.tsx

+            (fact) => String(fact.id) == selectedFactId,
+        );
+        if (selectedFact) {
+            //move the selected fact to the viewpointFactList


When writing mutations that reply on previous state it is not recommended to take the state variable as they do not update until re-rendered, which is not triggered immediately and instead can be batched. The current method results in race conditions and using useState(prev => [...prev, new]) is recommended

burnedinthesky · 2024-12-07T06:27:26Z

src/components/AuthorViewpoint/FactListCard.tsx

+            );
+            setSearchData(newSearchData);
+        }
+    }, [selectedFactId, setSelectedFactId]);


Please fix the linter warning

burnedinthesky · 2024-12-07T06:28:34Z

src/components/AuthorViewpoint/FactListCard.tsx

+        const newSelectedFacts = viewpointFactList.filter(
+            (fact) => String(fact.id) !== factId,
+        );
+        setViewpointFactList(newSelectedFacts);


The same applies here, consider basing filters on data passed in by the setState hook instead of relying on the state variable

burnedinthesky · 2024-12-07T06:28:49Z

src/components/AuthorViewpoint/FactListCard.tsx

+        );
+        setSearchData(newSearchData);
+
+        console.log(


Unnecessary logging.

yukicoder0509 requested a review from burnedinthesky November 21, 2024 06:15

feat: add metadata

af654da

vercel bot deployed to Preview November 23, 2024 02:23 View deployment

burnedinthesky requested changes Nov 25, 2024

View reviewed changes

refactor: using mantine input component

0e58d7b

vercel bot deployed to Preview November 25, 2024 15:13 View deployment

refactor: change html button to mantine button

6b1460b

vercel bot deployed to Preview November 26, 2024 09:17 View deployment

yukicoder0509 added 2 commits November 26, 2024 18:52

refactor: change html button to mantine button

02d95cb

refactor: change html button to mantine button

a8732a4

vercel bot deployed to Preview November 26, 2024 10:56 View deployment

yukicoder0509 requested a review from burnedinthesky November 26, 2024 10:56

burnedinthesky requested changes Nov 26, 2024

View reviewed changes

yukicoder0509 added 2 commits November 28, 2024 13:46

feat: using editible div to add padding between paragraph

034134c

feat: add place holder and space between paragraph

da39ce3

vercel bot deployed to Preview November 28, 2024 16:58 View deployment

yukicoder0509 requested a review from burnedinthesky November 28, 2024 16:59

burnedinthesky reviewed Dec 2, 2024

View reviewed changes

src/app/issues/[id]/author/page.tsx Outdated

};

};

type MetadataProps = {

Copy link

Contributor

burnedinthesky Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused props type

burnedinthesky requested changes Dec 2, 2024

View reviewed changes

feat: enable adding and removing facts

bf50d60

vercel bot had a problem deploying to Preview December 5, 2024 07:15 Failure

feat: delete all content when pressing delete button

a1e1793

vercel bot deployed to Preview December 5, 2024 09:03 View deployment

Merge branch 'development' of https://github.com/commonground-project…

7b1c094

…/frontend into author-viewpoint

vercel bot deployed to Preview December 5, 2024 09:18 View deployment

yukicoder0509 requested a review from burnedinthesky December 5, 2024 09:19

fix: disable the publish button if the content is empty

f76cb0c

vercel bot deployed to Preview December 5, 2024 12:33 View deployment

burnedinthesky reviewed Dec 7, 2024

View reviewed changes

burnedinthesky requested changes Dec 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish author viewpoint page #11

Finish author viewpoint page #11

yukicoder0509 commented Nov 21, 2024

vercel bot commented Nov 21, 2024 •

edited

Loading

burnedinthesky commented Nov 25, 2024

burnedinthesky left a comment

burnedinthesky Dec 2, 2024

burnedinthesky left a comment

burnedinthesky Dec 2, 2024

burnedinthesky Dec 2, 2024

burnedinthesky Dec 2, 2024

burnedinthesky Dec 2, 2024

burnedinthesky Dec 7, 2024

burnedinthesky Dec 7, 2024

burnedinthesky Dec 7, 2024

yukicoder0509 Dec 8, 2024

burnedinthesky Dec 7, 2024

burnedinthesky Dec 7, 2024

burnedinthesky Dec 7, 2024

burnedinthesky Dec 7, 2024

yukicoder0509 Dec 8, 2024

burnedinthesky Dec 7, 2024

burnedinthesky Dec 7, 2024 •

edited

Loading

burnedinthesky Dec 7, 2024

burnedinthesky Dec 7, 2024

burnedinthesky Dec 7, 2024

Finish author viewpoint page #11

Are you sure you want to change the base?

Finish author viewpoint page #11

Conversation

yukicoder0509 commented Nov 21, 2024

vercel bot commented Nov 21, 2024 • edited Loading

burnedinthesky commented Nov 25, 2024

burnedinthesky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burnedinthesky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burnedinthesky Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Nov 21, 2024 •

edited

Loading

burnedinthesky Dec 7, 2024 •

edited

Loading